Repository navigation
fix: working runner token exchange + OSC_REFRESH_TOKEN (stage 1 of #39) - #41
Merged
Merged
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Research Review
Verified
- Request shape:
POST /runner-token/refreshtakes JSON{ token }and returns{ token, expiresIn }. I confirmed this in osaas-token-servicesrc/model.ts(RunnerRefreshRequestandRunnerRefreshResponse). The route is registered insrc/api_service_token.ts:549. The PR's body, header and.tokenparsing match. - Reference implementation: web-runner
scripts/docker-entrypoint.sh(main) sends the sameContent-Type: application/jsonrequest with--data @filefrom a mode-600 temp file, so the PR follows the existing pattern. jqabsence: the PR parses withnode, as the architect note on #39 directed. The test stubsjqwith exit 127 and the success case still passes.grep jq Dockerfilefinds nothing.- Stage split matches birme's decision on #39 (comment 6719183061). The shell
OSC_ACCESS_TOKENis never assigned; the fresh token is set inline on theconfig-to-envcommand only. I checked lines 236-278 of the PR head: the export attribute never reaches the app, andOSC_REFRESH_TOKENis exported. - Tests: I ran every
tests/*.shon the PR head and all pass, including the new one (9/9) and the existing config-load-failure suite (11/11). The repo has no PR CI that runs tests, sogh pr checksreports nothing. set -esafety: every new command that can fail (curl,node) is guarded by|| var=..., so no new fatal path is added. The config load remains the only fatal path.- Redaction:
OSC_REFRESH_TOKENandconfig_tokenwere added toredact_config_log. The warning line prints only the status and a sanitisedcode. ## Lessonsis present and filled in.
Adjacent Stubs Detected
- None. I grepped
scripts/docker-entrypoint.shforundefined : undefined,|| undefined, TODO/FIXME/HACK and empty catch blocks and found no stubs in the touched code. The remaining|| trueuses are on unrelatedcatcalls.
Orphaned Files Detected
- None. The entrypoint is the Dockerfile entry point. The test follows the
tests/*.shconvention. README is documentation.
Escape Hatch Coverage
- The diff adds no terminal-rejection branch. An exchange failure only warns and falls back to the original token. Passing-path and failure-path tests both exist (
tests/test-entrypoint-runner-token-refresh.sh).
Object Shape Verification
- No spreads of fetched objects. The only parse is
JSON.parseon the real response shape, and it fails closed.
Contract diff
- No request handler changed. Files checked:
scripts/docker-entrypoint.sh,README.md,tests/test-entrypoint-runner-token-refresh.sh. - Container-level contract (the app environment):
OSC_ACCESS_TOKENstays the byte-identical 365-day token.OSC_REFRESH_TOKENis a new optional variable, which is an additive change. A repo-wide search found no existing consumer of the name, and ADR-0005 uses the same name. Stage 2, which changes the app-visible token, is explicitly not in this PR. Birme's staged decision is on #39: #39 (comment) - Timeout:
curl --max-time 10on the startup path, not a request path. Measurement is cited on #39 (dev p50 36 ms, max 133 ms; prod p50 34 ms, max 38 ms). The timeout branch falls back to the original token and continues, so it never becomes a terminal failure. Worst-case added boot delay is 10 s, and only if token-service hangs.
Impact Analysis
- Blast radius: python-runner entrypoint only. This runs for any app with
OSC_ACCESS_TOKENandCONFIG_SVCset. - Downstream consumers: apps read the unchanged
OSC_ACCESS_TOKEN. A parameter-store key namedOSC_REFRESH_TOKENoverrides the new variable, and the README documents this. - Coordinated changes needed: none. A runner image release is a human call, per the PR body. Sibling tickets (wasm, golang, dotnet, python-job-runner) are separate.
- Breaking change risk: low.
Risks Not Tested
- Live exchange against a real token-service with a genuine runner-refresh token was not run. The tests use a
curlstub. The request shape was checked against the source and web-runner, not against a live 200. - Behaviour of
@osaas/cli config-to-envwith a 1 h PAT was not tested. Web-runner already relies on the same behaviour. - The built image was not inspected for
nodeorjq. The check was based on the Dockerfile, so this is unverified at runtime. - Dev verification after merge (remake plus pod log) is still pending.
Research Suggestions
- After release, verify the
[CONFIG]warning does not appear in a dev pod log. This is the real-world confirmation of the exchange.
Verdict
No blocking issues found.
Reviewed by PR Research Validator. This review checks whether code was based on verified sources rather than assumptions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #39
Stage 1 of the decision in #39 (comment). Stage 2 is NOT in this PR and #39 stays open for it: making the app-visible
OSC_ACCESS_TOKENthe 1h PAT ships together with theREJECT_RUNNER_REFRESH_TOKENSrollout.What changed
scripts/docker-entrypoint.sh: the refresh exchange now POSTs{"token": "<jwt>"}(JSON body from a mode-600 temp file,Content-Type: application/json,--max-time 10) to/runner-token/refresh, as web-runner does.config-to-envis run with the fresh 1h PAT set inline on that command only.OSC_ACCESS_TOKENis never assigned, so the app still receives the unchanged 365-day token. The app additionally getsOSC_REFRESH_TOKEN(the original token, unless already set).node(nojqin the image); an empty parse result counts as an exchange failure.|| trueis replaced by a[CONFIG] WARNING: runner token exchange failed (HTTP <status>, code=<code>); using the original tokenline (status and code only, no body, no token). No new failure mode: the config load stays the only fatal path.OSC_REFRESH_TOKENand the fresh token are added toredact_config_log.OSC_REFRESH_TOKENwith a Python snippet.Tests
New
tests/test-entrypoint-runner-token-refresh.sh(9 cases, with ajqstub that exits 127): success assertsnpxsaw the fresh token, the app-visibleOSC_ACCESS_TOKENis unchanged andOSC_REFRESH_TOKENis set; failures (401 expired, 401 invalid, 500, curl exit 22 and 28, 200 without token, 200 non-JSON) assert warning, original token used, continues, no token in stderr. Alltests/*.shrun locally and pass (no PR CI runs tests in this repo).Changes for existing consumers
Nothing observable: the app's
OSC_ACCESS_TOKENis byte-identical to today (the assignment is inline on theconfig-to-envcommand, never in the shell);OSC_REFRESH_TOKENis a new additive variable (a repo-wide search by the architect found no other use; a parameter store key of that name overrides it).What a human does next: a runner image rebuild/release is birme's call; nothing else for this stage.
Lessons
The entrypoint's shell assignment
OSC_ACCESS_TOKEN=...keeps the export attribute, so assigning the exchange result would silently have shipped Stage 2; setting it inline on theconfig-to-envcommand avoids that. The existing harnesscurlstub exits 22, so every pre-existing test already runs through the new failure path.🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com